gh-96030: IsolatedAsyncioTestCase: don't create asyncio runner for skipped tests #96031
Conversation
tiran
commented
Aug 16, 2022
•
tiran
commented
- Issue: IsolatedAsyncioTestCase calls _setupAsyncioRunner even for skipped tests #96030
…for skipped tests
| def debug(self): | ||
| self._setupAsyncioRunner() | ||
| super().debug() | ||
| self._tearDownAsyncioRunner() |
There was a problem hiding this comment.
this used to not be called on an exception in debug() you probably want a test for this new behavior ?
There was a problem hiding this comment.
Also I think you fixed this #96021
There was a problem hiding this comment.
What do you mean by "this"? The function call self._tearDownAsyncioRunner()? It still does not get called when the setup or test case fails. It now gets called when asyncTearDown or tearDown fail with an exception. I think it's an acceptable deviation. Exceptions in tear down code are rare and it's ok to clean up the runner on error.
There was a problem hiding this comment.
Right but it used to not get called when debug failed, and now it does? So I think you fixed two bugs
| self._asyncioTestContext.run(self.setUp) | ||
| self._callAsync(self.asyncSetUp) | ||
| except BaseException: | ||
| # _callTearDown is not called when _callSetUp fails. |
There was a problem hiding this comment.
The problem is that doCleanups() is called in run() if _callSetUp() fails. It will try to call callbacks after closing the runner.
|
This is an interesting approach, and I would prefer a code that overrides methods like |
|
My approach addresses the fact that the current code does too much when a test is skipped. Let's merge your simple fix to unblock main and somebody else can take over my PR fix the code properly. |
|
How does this compare to #96033? It seems that is meant to fix the same issue. |
| try: | ||
| self._asyncioTestContext.run(self.setUp) | ||
| self._callAsync(self.asyncSetUp) | ||
| except BaseException: |
There was a problem hiding this comment.
Hm, I don't like catching BaseException here. It seems to only coincidentally fix the underlying issue -- which IMO is that the asyncio runner is set up even for skipped tests.
And the duplication of calling _tearDownAsyncioRunner both here and in _callTearDown also jars.
There was a problem hiding this comment.
Ah yes this means if you have an IsolatedAsyncioTestCase with only skipped tests it means set_event_loop(None) is called when you might not have expected it to be
|
This PR appears doomed. @tiran want to just close it? |
|
The original issue was fixed, and it is a proper solution. The code can be made more "elegant" if use the abstarct idea of this PR. I tried to implement it, but the simple and clear implementation of it free from the flaws of this PR suffered from two other issues:
It is possible to fix these two issues also, but the resulting code is far from "elegant". So for now I'm happy with the current code. |
So this issue is good to close? |
|
Closing per Serhiy's recommendation. |
